Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(ecs-patterns): Add LoadBalancer and TaskDefinition properties #4213

Merged
merged 6 commits into from
Sep 30, 2019
Merged

feat(ecs-patterns): Add LoadBalancer and TaskDefinition properties #4213

merged 6 commits into from
Sep 30, 2019

Conversation

piradeepk
Copy link
Contributor

@piradeepk piradeepk commented Sep 24, 2019 •
edited
Loading

Add LoadBalancer and TaskDefinition properties to be input to the constructor of all patterns.

BREAKING CHANGES:

  • In order to support TaskDefinition as an input property, moved all properties that are used to create a TaskDefinition into it's own props object (TaskImageOptions) and accept that as input to all ecs-patterns constructs.
  • Only one of TaskDefinition or TaskImageOptions can be passed as input to the constructs. Previously image was required, where as now image or taskDefinition is required, and having both will throw an exception.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@piradeepk piradeepk requested a review from SoManyHs as a code owner September 24, 2019 00:53
@mergify
Copy link
Contributor

mergify bot commented Sep 24, 2019

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

  • PR title type(scope): text
    • type: fix, feat, refactor go into CHANGELOG, chore is hidden
    • scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
    • text: use all lower-case, do not end with a period, do not include issue refs
  • PR Description
    • Rationale: describe rationale of change and approach taken
    • Issues: indicate issues fixed via: fixes #xxx or closes #xxx
    • Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
  • Testing
    • Unit test added. Prefer to add a new test rather than modify existing tests
    • CLI or init templates change? Re-run/add CLI integration tests
  • Documentation
    • README: update module README to describe new features
    • API docs: public APIs must be documented. Copy from official AWS docs when possible
    • Design: for significant features, follow design process

@piradeepk piradeepk changed the title (aws-ecs-patterns): Add LoadBalancer and TaskDefinition properties feat(ecs-patterns): Add LoadBalancer and TaskDefinition properties Sep 24, 2019
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@piradeepk piradeepk added the pr/do-not-merge This PR should not be merged at this time. label Sep 24, 2019
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot dismissed kohidave’s stale review September 25, 2019 00:49

Pull request has been modified.

kohidave
kohidave previously approved these changes Sep 25, 2019
View reviewed changes
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@piradeepk piradeepk requested a review from SoManyHs September 25, 2019 08:51
@piradeepk piradeepk removed the pr/do-not-merge This PR should not be merged at this time. label Sep 25, 2019
@mergify
Copy link
Contributor

mergify bot commented Sep 25, 2019

Thank you for contributing! Your pull request is now being automatically merged.

@mergify mergify bot dismissed kohidave’s stale review September 25, 2019 18:12

Pull request has been modified.

@piradeepk piradeepk requested a review from kohidave September 25, 2019 18:14
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

1 similar comment
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

rix0rrr
rix0rrr reviewed Sep 26, 2019
View reviewed changes
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also (re)moved a bunch of properties right? Please add a BREAKING CHANGES section to your PR description.

@rix0rrr rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Sep 26, 2019
@piradeepk
Copy link
Contributor Author

Updated description to describe breaking changes.

@piradeepk piradeepk assigned rix0rrr and unassigned kohidave and SoManyHs Sep 26, 2019
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@piradeepk
Copy link
Contributor Author

Resolved merge conflicts

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@rix0rrr rix0rrr merged commit f2a6d46 into aws:master Sep 30, 2019
@piradeepk piradeepk mentioned this pull request Oct 3, 2019
Closed
2 tasks
@SoManyHs SoManyHs mentioned this pull request Oct 3, 2019
Closed
5 tasks
This was referenced Oct 3, 2019
Closed
Closed
@hoegertn
Copy link
Contributor

hoegertn commented Oct 3, 2019

@pkandasamy91 Is it possible that no changes to https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-ecs-patterns/README.md were made to reflect the usage changes?

@netroy netroy mentioned this pull request Oct 14, 2019
Merged
@SoManyHs SoManyHs mentioned this pull request Nov 9, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rix0rrr rix0rrr rix0rrr left review comments

@SoManyHs SoManyHs Awaiting requested review from SoManyHs

@kohidave kohidave Awaiting requested review from kohidave

Assignees

@rix0rrr rix0rrr

Labels
pr/do-not-merge This PR should not be merged at this time.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@piradeepk @aws-cdk-automation @hoegertn @rix0rrr @kohidave @SoManyHs